fix: mempool lane size check on `CheckTx` #561

aljo242 · 2024-07-02T18:58:16Z

Closes BLO-1473

codecov · 2024-07-02T19:02:58Z

Codecov Report

Attention: Patch coverage is 65.64885% with 45 lines in your changes missing coverage. Please review.

Project coverage is 45.06%. Comparing base (242fdf2) to head (50cd6dc).

Files	Patch %	Lines
abci/checktx/mempool_parity_check_tx.go	69.84%	16 Missing and 3 partials ⚠️
testutils/utils.go	75.00%	4 Missing and 4 partials ⚠️
x/auction/types/mocks/bank_keeper.go	14.28%	5 Missing and 1 partial ⚠️
x/auction/types/mocks/account_keeper.go	33.33%	1 Missing and 1 partial ⚠️
x/auction/types/mocks/distribution_keeper.go	33.33%	2 Missing ⚠️
x/auction/types/mocks/rewards_address_provider.go	33.33%	1 Missing and 1 partial ⚠️
x/auction/types/mocks/staking_keeper.go	33.33%	2 Missing ⚠️
block/base/match.go	0.00%	1 Missing ⚠️
block/types.go	0.00%	1 Missing ⚠️
lanes/terminator/lane.go	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #561      +/-   ##
==========================================
+ Coverage   43.59%   45.06%   +1.46%     
==========================================
  Files          63       63              
  Lines        2812     2920     +108     
==========================================
+ Hits         1226     1316      +90     
- Misses       1459     1469      +10     
- Partials      127      135       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

davidterpay · 2024-07-02T20:19:05Z

abci/checktx/mempool_parity_check_tx.go

+// GetContextForTx is returns the latest committed state and sets the context given
+// the checkTx request.
+func (m MempoolParityCheckTx) GetContextForTx(req *cmtabci.RequestCheckTx) sdk.Context {
+	// Retrieve the commit multi-store which is used to retrieve the latest committed state.
+	ms := m.baseApp.CommitMultiStore().CacheMultiStore()
+
+	// Create a new context based off of the latest committed state.
+	header := cmtproto.Header{
+		Height:  m.baseApp.LastBlockHeight(),
+		ChainID: m.baseApp.ChainID(),
+	}
+	ctx, _ := sdk.NewContext(ms, header, true, m.baseApp.Logger()).CacheContext()
+
+	// Set the context to the correct checking mode.
+	switch req.Type {
+	case cmtabci.CheckTxType_New:
+		ctx = ctx.WithIsCheckTx(true)
+	case cmtabci.CheckTxType_Recheck:
+		ctx = ctx.WithIsReCheckTx(true)
+	default:
+		panic("unknown check tx type")
+	}
+
+	// Set the remaining important context values.
+	ctx = ctx.
+		WithTxBytes(req.Tx).
+		WithEventManager(sdk.NewEventManager()).
+		WithConsensusParams(m.baseApp.GetConsensusParams(ctx))
+
+	return ctx
+}


can you benchmarket how long this takes just so we have a reference on the slowdown we expected? I imagine some of this is not necessary.

How would you imagine benchmarking this?

Start a small in mem test app and call this on it?

benchmarking with a real simapp would be better, but probably not important to get this shipped. fine as is

abci/checktx/mempool_parity_check_tx.go

davidterpay · 2024-07-02T20:23:25Z

abci/checktx/mempool_parity_check_tx.go

+		sdkCtx := m.GetContextForTx(req)
+		lane, err := m.matchLane(sdkCtx, tx)
+		if err != nil {
+			m.logger.Debug("failed to match lane", "lane", lane, "err", err)
+			return sdkerrors.ResponseCheckTxWithEvents(
+				err,
+				0,
+				0,
+				nil,
+				false,
+			), nil
+		}
+
+		consensusParams := sdkCtx.ConsensusParams()
+		laneSize := lane.GetMaxBlockSpace().MulInt64(consensusParams.GetBlock().GetMaxBytes()).TruncateInt64()
+
+		txSize := int64(len(req.Tx))
+		if txSize > laneSize {
+			m.logger.Debug(
+				"tx size exceeds max block bytes",
+				"tx", tx,
+				"tx size", txSize,
+				"max bytes", laneSize,
+			)
+
+			return sdkerrors.ResponseCheckTxWithEvents(
+				fmt.Errorf("tx size exceeds max block bytes"),
+				0,
+				0,
+				nil,
+				false,
+			), nil
+		}


Does it make sense to do this check before the entire ante-handler sequence is run?

I'm not sure - I think the case with a huge Tx is much less likely than the case where youre Tx just fails the antehandler. So i would rather run that and have the matching logic done later only when we need to

abci/checktx/mempool_parity_check_tx.go

Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com>

nivasan1

lgtm

* push * init * fix setup * format * fix test * use lane * ok * finalize * fix everything * lint fix: * Update abci/checktx/mempool_parity_check_tx.go Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com> * lint fix * tidy * remove * cleanup --------- Co-authored-by: David Terpay <david.terpay@gmail.com> Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com> (cherry picked from commit f1cde2a) # Conflicts: # README.md # abci/abci.go # abci/checktx/check_tx_test.go # abci/checktx/mempool_parity_check_tx.go # block/mocks/lane.go # block/mocks/lane_mempool.go # go.mod # go.sum # tests/app/app.go # tests/app/testappd/cmd/testnet.go # x/auction/types/mocks/bank_keeper.go # x/auction/types/mocks/staking_keeper.go

* push * init * fix setup * format * fix test * use lane * ok * finalize * fix everything * lint fix: * Update abci/checktx/mempool_parity_check_tx.go Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com> * lint fix * tidy * remove * cleanup --------- Co-authored-by: David Terpay <david.terpay@gmail.com> Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com> (cherry picked from commit f1cde2a) # Conflicts: # README.md # go.mod # go.sum # x/auction/types/mocks/account_keeper.go # x/auction/types/mocks/bank_keeper.go # x/auction/types/mocks/distribution_keeper.go # x/auction/types/mocks/rewards_address_provider.go # x/auction/types/mocks/staking_keeper.go

* fix: mempool lane size check on `CheckTx` (#561) * push * init * fix setup * format * fix test * use lane * ok * finalize * fix everything * lint fix: * Update abci/checktx/mempool_parity_check_tx.go Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com> * lint fix * tidy * remove * cleanup --------- Co-authored-by: David Terpay <david.terpay@gmail.com> Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com> (cherry picked from commit f1cde2a) # Conflicts: # README.md # go.mod # go.sum # x/auction/types/mocks/account_keeper.go # x/auction/types/mocks/bank_keeper.go # x/auction/types/mocks/distribution_keeper.go # x/auction/types/mocks/rewards_address_provider.go # x/auction/types/mocks/staking_keeper.go * fix * tidy --------- Co-authored-by: Alex Johnson <alex@skip.money>

* fix: mempool lane size check on `CheckTx` (#561) * push * init * fix setup * format * fix test * use lane * ok * finalize * fix everything * lint fix: * Update abci/checktx/mempool_parity_check_tx.go Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com> * lint fix * tidy * remove * cleanup --------- Co-authored-by: David Terpay <david.terpay@gmail.com> Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com> (cherry picked from commit f1cde2a) # Conflicts: # README.md # abci/abci.go # abci/checktx/check_tx_test.go # abci/checktx/mempool_parity_check_tx.go # block/mocks/lane.go # block/mocks/lane_mempool.go # go.mod # go.sum # tests/app/app.go # tests/app/testappd/cmd/testnet.go # x/auction/types/mocks/bank_keeper.go # x/auction/types/mocks/staking_keeper.go * bettington --------- Co-authored-by: Alex Johnson <alex@skip.money>

aljo242 and others added 9 commits July 2, 2024 12:49

push

beb7aa4

Merge branch 'main' into fix/lane-size-bound

e9adb18

init

fa30deb

fix setup

69d83b3

format

1ccb70b

fix test

517d71e

use lane

357bc89

ok

57aaa3b

finalize

4bf0a4a

aljo242 added backport/v1.x.x Backport your PR to the v1.x.x release backport/v2.x.x Backport your PR to the v2.x.x release labels Jul 2, 2024

aljo242 self-assigned this Jul 2, 2024

aljo242 requested review from davidterpay, nivasan1 and Eric-Warehime as code owners July 2, 2024 18:58

aljo242 added 3 commits July 2, 2024 15:20

fix everything

e78e34e

lint fix:

d1dec20

Merge branch 'main' into fix/lane-size-bound

4d1eedf

davidterpay reviewed Jul 2, 2024

View reviewed changes

abci/checktx/mempool_parity_check_tx.go Outdated Show resolved Hide resolved

davidterpay reviewed Jul 2, 2024

View reviewed changes

davidterpay approved these changes Jul 2, 2024

View reviewed changes

nivasan1 reviewed Jul 2, 2024

View reviewed changes

abci/checktx/mempool_parity_check_tx.go Show resolved Hide resolved

abci/checktx/mempool_parity_check_tx.go Show resolved Hide resolved

aljo242 and others added 5 commits July 2, 2024 17:48

Update abci/checktx/mempool_parity_check_tx.go

53649a7

Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com>

lint fix

4810c2c

tidy

59b2e4f

remove

e324323

cleanup

50cd6dc

nivasan1 approved these changes Jul 2, 2024

View reviewed changes

aljo242 merged commit f1cde2a into main Jul 2, 2024
11 of 12 checks passed

aljo242 deleted the fix/lane-size-bound branch July 2, 2024 23:19

This was referenced Jul 2, 2024

fix: mempool lane size check on CheckTx (backport #561) #564

Merged

fix: mempool lane size check on CheckTx (backport #561) #565

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: mempool lane size check on `CheckTx` #561

fix: mempool lane size check on `CheckTx` #561

aljo242 commented Jul 2, 2024

codecov bot commented Jul 2, 2024 •

edited

Loading

davidterpay Jul 2, 2024

aljo242 Jul 2, 2024

davidterpay Jul 2, 2024

davidterpay Jul 2, 2024

aljo242 Jul 2, 2024

nivasan1 left a comment

fix: mempool lane size check on CheckTx #561

fix: mempool lane size check on CheckTx #561

Conversation

aljo242 commented Jul 2, 2024

codecov bot commented Jul 2, 2024 • edited Loading

Codecov Report

davidterpay Jul 2, 2024

Choose a reason for hiding this comment

aljo242 Jul 2, 2024

Choose a reason for hiding this comment

davidterpay Jul 2, 2024

Choose a reason for hiding this comment

davidterpay Jul 2, 2024

Choose a reason for hiding this comment

aljo242 Jul 2, 2024

Choose a reason for hiding this comment

nivasan1 left a comment

Choose a reason for hiding this comment

fix: mempool lane size check on `CheckTx` #561

fix: mempool lane size check on `CheckTx` #561

codecov bot commented Jul 2, 2024 •

edited

Loading